Skip to content
This repository has been archived by the owner on Apr 28, 2020. It is now read-only.

Validate hostname in cloud-init settings #543

Merged
merged 1 commit into from
Aug 21, 2019

Conversation

pcbailey
Copy link
Contributor

This PR adds hostname validation to the cloud-init settings section of the create VM/template wizard.

Bug: https://bugzilla.redhat.com/show_bug.cgi?id=1648321

@coveralls
Copy link

coveralls commented Aug 19, 2019

Pull Request Test Coverage Report for Build 2270

  • 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.003%) to 82.846%

Totals Coverage Status
Change from base Build 2261: 0.003%
Covered Lines: 4165
Relevant Lines: 4814

💛 - Coveralls

@@ -51,6 +52,7 @@ const validateResolver = {
[IMAGE_URL_KEY]: asVmSettingsValidator(validateURL),
[PROVIDER_KEY]: validateProviderDropdown,
[MEMORY_KEY]: asVmSettingsValidator(validateMemory),
[HOST_NAME_KEY]: asVmSettingsValidator(validateVmLikeEntityName),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think host name should have the same validation as VM name

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. I'll create a separate function for it. I think for the moment all that's needed is validateDNS1123SubdomainValue, correct?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

y I guess, that should be enough

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@pcbailey pcbailey force-pushed the validate-cloud-init-hostname branch from 95ba78f to b00ad03 Compare August 20, 2019 21:07
@@ -123,6 +123,11 @@ export const validateVmLikeEntityName = (value, vmSettings, props) => {
);
};

export const validateCloudInitHostName = value => {
const dnsValidation = validateDNS1123SubdomainValue(value);
return dnsValidation && dnsValidation.type === VALIDATION_ERROR_TYPE ? dnsValidation : null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we have to check for the error_type here, because all the errors will be error type or none in this case. Nevertheless I do think we should check for empty error here, because the host name can be empty if I am not mistaken.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're correct. Done!

@pcbailey pcbailey force-pushed the validate-cloud-init-hostname branch from b00ad03 to 74053ff Compare August 21, 2019 12:31
@@ -123,6 +123,11 @@ export const validateVmLikeEntityName = (value, vmSettings, props) => {
);
};

export const validateCloudInitHostName = value => {
const dnsValidation = validateDNS1123SubdomainValue(value);
return dnsValidation && dnsValidation.type !== EMPTY_ERROR ? dnsValidation : null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a bit differently

Suggested change
return dnsValidation && dnsValidation.type !== EMPTY_ERROR ? dnsValidation : null;
return dnsValidation && dnsValidation.isEmptyError ? null : dnsValidation;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks much cleaner, thanks. =) I've updated the commit.

@pcbailey pcbailey force-pushed the validate-cloud-init-hostname branch from 74053ff to 3a97559 Compare August 21, 2019 13:58
@atiratree atiratree merged commit 77a6d9c into kubevirt:master Aug 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants